-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adding docs for Unsafe Apis in System.Runtime.CompilerServices #2819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -1062,8 +1066,7 @@ | |||
<Docs> | |||
<typeparam name="T">The type of reference.</typeparam> | |||
<param name="source">The reference to subtract the offset from.</param> | |||
<param name="bytesOffset">The offset to subtract.</param> | |||
<param name="byteOffset">To be added.</param> | |||
<param name="byteOffset">The offset to subtract.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed one of the parameters from here. I suspect it was a duplicate. This happens when the parameter name does not match in the source file and the ref file. The official one is the one in the ref file. Which one is the correct one, can you please confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes byteOffset is the name of parameter in the ref file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is interesting because the Parameters section, signatures, etc. only shows one of them.
@joelmartinez is this a bug in mdoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered there's another possible case: if the parameter name is changed from one version to another, they both show up, and both need to be documented, not just one.
@joelmartinez may know the true answer for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue should be resolved in the next mdoc release (5.7.4.10) ... ETA is soon, but not immediately, so manual interventions for this is ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of this too but then the parameter would have additional information to convey this like here where you see different frameworkalternate values for each parameter:
https://github.com/dotnet/dotnet-api-docs/blob/master/xml/Microsoft.VisualBasic/Collection.xml#L1161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this change. I left a few comments. Can you please take a look? Let's also wait for Maira/Ron to jump in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments but looks good overall @Anipik!
@@ -1062,8 +1066,7 @@ | |||
<Docs> | |||
<typeparam name="T">The type of reference.</typeparam> | |||
<param name="source">The reference to subtract the offset from.</param> | |||
<param name="bytesOffset">The offset to subtract.</param> | |||
<param name="byteOffset">To be added.</param> | |||
<param name="byteOffset">The offset to subtract.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is interesting because the Parameters section, signatures, etc. only shows one of them.
@joelmartinez is this a bug in mdoc?
Co-Authored-By: Maira Wenzel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good now, @Anipik. Thanks for making the change.
@mairaw if it looks good to you, we can get this merged.
Pending (but not blocking) @joelmartinez 's help to determine why there was a parameter vs param name mismatch.
cc @carlossanlop @rpetrusha